Skip to content

fix(ci): correctly download npm/docker artifacts #4995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Mar 17, 2022
Merged

fix(ci): correctly download npm/docker artifacts #4995

merged 12 commits into from
Mar 17, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Mar 15, 2022 •
edited
Loading

Context

This PR fixes the npm and docker workflows that runs on a release to correctly download artifacts so it can publish it to the npm and Docker registries.

https://github.com/coder/code-server/blob/main/.github/workflows/npm-brew.yaml#L21-L37

Problem

The default download-artifact assumes that you're downloading artifacts from the running workflow. The problem is npm-brew.yaml runs on released events.

Example: maintainer creates release branch v4.1.0 ➡️ opens PR ➡️ team approves PR ➡️ maintainer merges PR into main ➡️ npm job from npm-brew.yaml and the docker.yaml workflows kick off.

Therefore this workflow is not associated with any PR or any commit, but rather a GitHub event.

Solution

Download npm-release artifact and release-packages from last successful run where branch is named v0.0.0 (with actual version) using this community GitHub Action dawidd6/action-download-artifact.

Other considerations

Why not use latest successful CI run for main?

There would be a race condition since by the time the release PR gets merged and main finishes running, this npm workflow would already have run, so the last successful CI run for main wouldn't be 1:1 with the release branch merge commit.

Why not use the same branch name each time like release?

@vapurrmaid and @code-asher brought up a good point. If we do this, then we have no way to differentiate between a 4.1.0 and a 4.2.0 release workflow since they both use the same branch name. This is important because there are times we need to republish releases like on Docker and we'd want to use the same build.

Testing Plan

  1. Fork cdr/code-server
  2. Checkout this branch: Release test jsjoeio/code-server#2
  3. Merge into main
  4. Create new branch called release
  5. Get CI to run and pass.
  6. Merge branch
  7. Create new GitHub release
  8. See if npm-brew runs and download-artifact works as expected: https://github.com/jsjoeio/code-server/runs/5562662740?check_suite_focus=true#step:3:16

Fixes #4949

@jsjoeio jsjoeio added the ci Issues related to ci label Mar 15, 2022
@jsjoeio jsjoeio self-assigned this Mar 15, 2022
@github-actions
Copy link

github-actions bot commented Mar 15, 2022 •
edited
Loading

✨ code-server docs for PR #4995 is ready! It will be updated on every commit.

@jsjoeio jsjoeio temporarily deployed to npm March 15, 2022 21:09 Inactive
@codecov
Copy link

codecov bot commented Mar 15, 2022 •
edited
Loading

Codecov Report

Merging #4995 (b27ad22) into main (a0561c7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4995   +/-   ##
=======================================
  Coverage   71.58%   71.58%           
=======================================
  Files          29       29           
  Lines        1675     1675           
  Branches      373      373           
=======================================
  Hits         1199     1199           
  Misses        405      405           
  Partials       71       71

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0561c7...b27ad22. Read the comment docs.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 15, 2022 •
edited
Loading

Testing Notes

Happy I tested this. The current approach didn't work 🤔

image

https://github.com/jsjoeio/code-server/runs/5562477153?check_suite_focus=true#step:3:11

Did some debugging with @code-asher - when we curl the GitHub API, we get some responses so we're not sure what's happening.

Success

This worked: https://github.com/jsjoeio/code-server/runs/5562662740?check_suite_focus=true#step:3:16

@jsjoeio jsjoeio marked this pull request as ready for review March 16, 2022 21:32
@jsjoeio jsjoeio requested a review from a team March 16, 2022 21:32
greyscaled
greyscaled reviewed Mar 16, 2022
View reviewed changes
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's a way that it can match branches like release-*:

with:
    branch: release-*

so that it doesn't rely on creating and deleting the same release branch over and over again.

🤔

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 16, 2022

I wonder if there's a way that it can match branches like release-*:

with:
    branch: release-*

so that it doesn't rely on creating and deleting the same release branch over and over again.

🤔

Doesn't seem like it from what I can tell, but there is an open PR from January: dawidd6/action-download-artifact#107 but it's only for artifact names :/

jsjoeio
jsjoeio commented Mar 16, 2022
View reviewed changes
Copy link
Contributor Author

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make changes

@jsjoeio jsjoeio requested a review from code-asher March 17, 2022 17:47
@jsjoeio jsjoeio changed the title fix(ci): correctly download npm artifact fix(ci): correctly download npm/docker artifacts Mar 17, 2022
code-asher
code-asher approved these changes Mar 17, 2022
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

@jsjoeio jsjoeio temporarily deployed to npm March 17, 2022 18:05 Inactive
@jsjoeio jsjoeio merged commit 5afb26f into main Mar 17, 2022
@jsjoeio jsjoeio deleted the 4949-chore-fix-npm-workflow branch March 17, 2022 18:52
TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022
@jsjoeio @TinLe
fix(ci): correctly download npm/docker artifacts (coder#4995)
60c3fc8 
* fix(ci): correctly download npm artifact

* fixup! fix(ci): correctly download npm artifact

* docs: update MAINTAINING

* fixup! docs: update MAINTAINING

* fixup! Merge branch 'main' into 4949-chore-fix-npm-workflow

* chore: get ci to run

* refactor: use vVERSION branch name instead of release

* refactor: use new download artifact in docker workflow

* refactor: clean up release-github-assets script

* fixup: remove extra v

* fixup! fixup: remove extra v
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greyscaled greyscaled greyscaled left review comments

@code-asher code-asher code-asher approved these changes

Assignees

@jsjoeio jsjoeio

Labels
ci Issues related to ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Chore]: fix npm workflow
3 participants
@jsjoeio @greyscaled @code-asher